Skip to content

fix: revert propagating schema extension node to router#2328

Merged
jensneuse merged 1 commit intomainfrom
david/eng-8464-revert-schema-ext-sdl-propagation
Nov 12, 2025
Merged

fix: revert propagating schema extension node to router#2328
jensneuse merged 1 commit intomainfrom
david/eng-8464-revert-schema-ext-sdl-propagation

Conversation

@Aenimus
Copy link
Copy Markdown
Member

@Aenimus Aenimus commented Nov 12, 2025

Summary by CodeRabbit

  • Improvements

    • Enhanced schema normalization with improved support for schema definitions and extensions.
    • Improved deprecated directive handling and availability in schema definitions.
  • Chores

    • Removed unused internal deprecation utilities.

Checklist

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 12, 2025

Walkthrough

This PR extends the normalization system to track and propagate schema definition/extension nodes through the normalization pipeline. It updates NormalizationSuccess with an optional schemaNode field, modifies the normalization factory's addSchemaDefinitionNode method to return the created node, removes unused deprecation-related imports, and updates tests to verify deprecated directive handling.

Changes

Cohort / File(s) Summary
Type definitions
composition/src/normalization/types.ts
Extended NormalizationSuccess to include optional schemaNode?: SchemaDefinitionNode | SchemaExtensionNode field; added SchemaDefinitionNode and SchemaExtensionNode imports from graphql.
Factory implementation
composition/src/v1/normalization/normalization-factory.ts
Changed #addSchemaDefinitionNode return type from void to SchemaDefinitionNode | SchemaExtensionNode | undefined; updated normalize and batchNormalize methods to capture and propagate returned schemaNode; added SchemaExtensionNode import; removed unused imports (DEPRECATED, REASON, DEFAULT_DEPRECATION_REASON, stringToNameNode); added schema extension handling logic.
Test updates
composition/tests/v1/directives/directives.test.ts
Added exports for DEPRECATED and DEPRECATED_DEFINITION from public API; updated test assertions to include deprecated directive mapping entry; added c: ID @deprecated`` field to test SDL; added skipped test case for directives after schema definitions/extensions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Extra attention areas:
    • Verify schemaNode propagation through normalize and batchNormalize call chains
    • Confirm schema extension handling logic correctly determines SchemaDefinitionNode vs SchemaExtensionNode creation
    • Validate test assertions align with deprecated directive mapping expectations and schema output

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: reverting schema extension node propagation to the router, which aligns with the removal of schemaNode from public results and the updates to normalization logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef084cb and d177530.

📒 Files selected for processing (3)
  • composition/src/normalization/types.ts (2 hunks)
  • composition/src/v1/normalization/normalization-factory.ts (4 hunks)
  • composition/tests/v1/directives/directives.test.ts (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.

Applied to files:

  • composition/tests/v1/directives/directives.test.ts
  • composition/src/v1/normalization/normalization-factory.ts
🧬 Code graph analysis (1)
composition/tests/v1/directives/directives.test.ts (3)
composition/src/types/types.ts (1)
  • DirectiveName (3-3)
composition/src/utils/string-constants.ts (1)
  • DEPRECATED (27-27)
composition/src/v1/constants/directive-definitions.ts (1)
  • DEPRECATED_DEFINITION (203-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: integration_test (./events)
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: Analyze (go)

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 12, 2025

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-84336d160f6bd08c45918a347456a1103bc9cd55

@jensneuse jensneuse merged commit dcb6b3b into main Nov 12, 2025
52 of 54 checks passed
@jensneuse jensneuse deleted the david/eng-8464-revert-schema-ext-sdl-propagation branch November 12, 2025 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants